Stop autosave from stalling the game at high speed - #260
Conversation
At 8x-40x the game froze for a fraction of a second every second or two. Every 256 ticks GameGUI::syncStep wrote the whole game, including every cached pathfinding gradient, to Auto_save.game. Late-game saves are 65-120 MB, and at 8x 256 ticks is about 1.3 seconds. - Write gradient fields as one run of bytes through OutputStream::writeUint16Sections. The bytes and SHA1 are the same as per-value writes, without a virtual call, name string and SHA1 update per value. - Give the atomic autosave writer a 1 MiB buffer instead of 16 KiB. - Skip the whole-file SHA1 for autosaves; the header keeps zeros. Engine::haveMap, the hash's only reader, now fetches the host's copy of any file without a hash instead of trusting a local file. - Scale the autosave interval with the game-speed preset so saves stay about 10 seconds of real time apart. Normal speed keeps ticks 79, 335, and so on. Counting from the last save also stops a soft-paused game on a save tick from autosaving every frame. Simulation checksums and replays are unchanged. At normal speed the autosave differs from master's only in its 20 SHA1 bytes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPPkz7Vd7QLMsbQfqy3kHU
Autosave serialized the game and wrote it to disk on the game thread. At late-game sizes (65-160 MB) the write alone took 30-70 ms, with spikes past 180 ms, on every autosave. - Serialize between ticks into memory, then hand the bytes to a new GAGCore::BackgroundFileWriter, which replaces the file through FileManager::writeAtomically on a worker thread. A snapshot that has not started writing is replaced by a newer one. The worker exists only while there is something to write, so waiting leaves no thread behind and a forked process never inherits one. - MemoryStreamBackend appends past its end instead of resizing, which zero-filled every byte before writing it, and gains reserve() and takeContents() so the snapshot moves to the writer without a copy. - Wait for a pending autosave before a session ends, before an in-game save, and when GameGUI is destroyed. - Add Settings > Gameplay > Autosave (autosaveGames, on by default), translated for every language. - Autosave now hands writeAtomically one complete buffer, which bypasses the stream buffer, so the 1 MiB atomic-write buffer from the previous commit no longer has any effect; revert it. Saved bytes, simulation checksums and replays are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPPkz7Vd7QLMsbQfqy3kHU
Two append/append conflicts: independent constant blocks in EngineTiming.h (autosave cadence here, gradient rebuild interval on master) and a duplicate Version.h include in SavegameSafetyHarness.cpp. Kept both constant blocks; kept the single existing Version.h include. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A5GEG5t9ithd47SFm6PTDZ
|
Merged current master ( Two files conflicted, both append/append:
No production logic was touched by the resolution. Verified locally on the merged head (macOS / Apple M3) before pushing:
CI is running on |
|
If savegames are up to 160MB, maybe we should save them differently? In a perfect world we could store replays with a timestamp but of course, every logic change breaks such savegames. We could also decide that gradients don't get saved and get computed first thing loading a savegame. That's probably besides the point of this PR but it would be the simpler change.
Why not compute the hash instead of downloading the map? It's 200ms to compute a 160MB hash but uploads from the host to 3 players ... takes much longer.
If that happens, something stops writing the savegame within 10s, so it might happen continuously. What happens if the prior has started writing but not ended? Will the next one skip and we drop to every 20s? I guess that would be a fair logic and probably never trigger.
Can't the thread saving the game store that hash? It's literally dirt cheap. If you threw away data needed to hash, it's ok to store a different hash as obviously the data is not needed to play the game. Just to verify it a map was distributed already, so the recipient should be able to hash and verify anyway ...
NACK |
…way round Autosaves had dropped their whole-file SHA1 to keep hashing off the game thread, which made a client joining from an autosave always download it. Game::save now records what the hash needs in a DeferredGameSHA1 and the BackgroundFileWriter applies it on its worker, so autosave bytes match an inline-hashed save again and Engine::haveMap is back to master's logic. MapHeader::operator== has compared the SHA1 inverted since 8cf81c9, so haveMap re-downloaded identical files and trusted different ones. Fixed, with a harness check that fails on the old comparison. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WePqccHKmAjY6cwoqEcvBE
|
@Giszmo — took your direction; Hash on the writer thread, no more download. Autosaves carry their SHA1 again. And the comparison it relies on is fixed here too. Overlapping saves. Nothing gets skipped. If a snapshot arrives while an earlier one is still being written, it waits and is written straight after the current write finishes. Only a snapshot that hasn't started is replaced by a newer one. So if writes were ever slower than 10 s, you'd get back-to-back writes of the latest state, not a drop to every 20 s. Each queued snapshot's hash step travels with it; the harness queues 50 and checks the last one lands with its own hash. Not saving gradients / saving differently. Agreed that's worth doing and would shrink these files a lot, but it's a save-format change with its own compatibility work, so I've left it out of this PR. Verified on macOS (Apple M3): client and server builds; |
At 8x, 13x and 40x the game runs smoothly for a second or two, then freezes for a fraction of a second, over and over. The freeze is the autosave. Every 256 ticks
GameGUI::syncStepserialized the whole game, including every cached pathfinding gradient, and wrote it toAuto_save.gameon the game thread. Late-game saves reach 65–160 MB, and at 8x 256 ticks is only about 1.3 seconds.Changes
OutputStream::writeUint16Sections. Each value used to cost three virtual calls, a name string the binary stream ignores, and its own 2-byte SHA1 update. The output bytes and SHA1 are identical;TextOutputStreamkeeps the per-value loop.Game::savecan leave the whole-file SHA1 for later, recording the hashed range and the header bytes as first written in aDeferredGameSHA1. TheBackgroundFileWriterapplies it on its worker just before the write, so an autosave's bytes, SHA1 included, are identical to an inline-hashed save, and the game thread does no hashing.MapHeader::operator==compares the SHA1 the right way round. It has been inverted since 8cf81c9 (2008):std::equal(...)==0was true only when the hashes differed.Engine::haveMap, its only caller, so re-downloaded identical files and trusted a different file with the same name, team count and map offset. A client joining a YOG game now keeps its local copy exactly when it matches the host's.GAGCore::BackgroundFileWriterthen replaces the file throughFileManager::writeAtomicallyon a worker thread.GameGUIis destroyed.MemoryStreamBackendappends past its end instead of resizing, which zero-filled every byte before writing it. It also gainsreserve()andtakeContents(), so the snapshot moves to the writer without a copy.autosaveGames, on by default), translated for every language.For review: behavior changes
Compatibility
gd-bigarena-longfor 20,000 ticks ends on master's checksum,5abcb33f. A 3,000-tick run ends on2a2689edwith autosave on and off.Auto_save.gameis byte-identical to the one this PR's previous commit wrote synchronously.Verification
All results are from this branch's final tree.
BufferedFileStreamHarnesspasses. It now also checks thatMemoryStreamBackendoverwrites, appends, zero-fills seek gaps and hands over its contents.SavegameSafetyHarnesspasses, with new checks:Engine::haveMaptrusts the saved file for its own header and rejects it when the host's SHA1 differs by one bittest/run-settings-tests.pyandtest/run-game-speed-tests.py --settings-onlypass.SettingsScreenTestchecks that the Autosave toggle persists off and back on.data/check_translations.py --strictgives the same report as before this change, andtest/test_translations.pypasses.scons -C testbuilds, andTestsRunner(187 tests),WinningConditionsHarnessandReplayStepCounterTestpass.operator==fails thehaveMapcheck; hashing the backpatched header instead of the recorded one fails the stale-offset check.Test games
games/gd-bigarena-long.game: Oazis, 256×256, 11 Castor/Warrush teams.GLOB2_TEST_SEED=7 glob2 -test-games-nox 1 --map Oazis --matchup nicowar,econo,nicowar,econo,nicowar,econo,nicowar,econo,nicowar,econo,castor --save-game-as <file>How the runs were done: release build,
glob2 --nox <game> <ticks> 1, with a disposable HOME whosepreferences.txtsetsgameSpeedand, for the on/off runs,autosaveGames.Game-thread stall per autosave
Method:
Limits
Auto_save.gamesurvives, because the file is only replaced by rename.test/run-settings-tests.py, which covers the new toggle. It passed locally.haveMapis covered directly in the harness, not through a live join.🤖 Generated with Claude Code
https://claude.ai/code/session_01LPPkz7Vd7QLMsbQfqy3kHU